-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Edit path when building sdist package #1687
Conversation
This patch changes the working directory from the temp to the project when building sdist package.This resolves issues with relative paths in configuration files. Resolves pypa#1683
8877de8
to
cc3fd8c
Compare
Nice one, thanks! Could you also add an integration test for this? It might be possible to add an assert to an existing test in https://github.com/pypa/cibuildwheel/blob/main/test/test_from_sdist.py |
@joerick TBH it's not exactly obvious what kind of check could be added into |
CIBW_BEFORE_BUILD runs from the CWD, so we could check we are in the project dir by doing something like |
UPD: adding tests may take a while since Olena is a beginner and needs to learn a thing or two about the ecosystem. I was helping her out to figure out how to run tests locally and learned that following the CONTRIBUTING instructions doesn't work — the way the tests are being executed requires something like @joerick have you considered synchronizing the CIs with the way the invocations are supposed to run locally to avoid such discrepancies? As in run nox everywhere the same (ish) way? One interesting approach I saw in some other projects was having a dedicated "dev env" CI job that essentially runs the tools in a way the contributors would do it locally. |
Apologies, that doc does probably need a note about this. I wouldn't recommend setting the GITHUB_ACTIONS like you mention, rather what I do is set I don't use the nox integration personally, because the test suite takes a while to run, so I generally want to be more specific. I will generally do Just one caveat to the above - for local runs with |
Interesting idea... though really the problem here is that the docs are incomplete, I don't think we can test for that. |
@joerick I checked the |
The idea with workflows tools like Note that both tools have ways to list the configured sessions. They are If the contributing doc says to check out the sessions in the workflow tool, with a few more concrete examples, and the CI runs the same, that's usually the best way of making sure there's no difference between what different people run. A more sophisticated solution could be auto-generating a portion of the contributing doc from the nox file. |
FYI, you can also use |
I've just pushed a PR #1698 with an improvement to this page. My opinion is that the direct pytest invocation is better, as I'm nearly always using some pytest-specific option, so it's extra level of indirection and mental overhead to also think about the Regarding using |
@joerick while I understand your sentiment, people are quite used to passing arguments to the wrapped commands. This is quite idiomatic in the tox world (and by extension, nox too). The whole idea is that this ends up always using the correct environment that is set up properly and the same way as in the CI. Tox also has As for the custom CLI options, a better place for them is pytest. I can take a look into that, if that's something you'd like to explore. |
The change extends the existing test — `test_simple`. It checks that the temporary wheel build directory is the project root extracted from the source distribution. It does so by testing the presence of the `setup.py` in the current working directory, as a side effect.
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
No CI failures so far 🤞 Waiting for the completion of the rest of the jobs. |
Nah, squash and merge is fine unless files both move and change. |
Thanks all for this! |
This patch changes the working directory from the temp to the project when building sdist package. This resolves issues with relative paths in configuration files.
Resolves #1683